Skip to content

Add support for Broadlink BG1 devices#33399

Closed
b4dpxl wants to merge 3 commits intohome-assistant:devfrom
b4dpxl:broadlink-bg1
Closed

Add support for Broadlink BG1 devices#33399
b4dpxl wants to merge 3 commits intohome-assistant:devfrom
b4dpxl:broadlink-bg1

Conversation

@b4dpxl
Copy link
Copy Markdown
Contributor

@b4dpxl b4dpxl commented Mar 29, 2020 •
edited
Loading

Proposed change

This adds support for the British General Broadlink BG1 switches.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
switch:
- platform: custom_broadlink
  host: 192.168.0.123
  mac: "24:df:a7:aa:bb:cc"
  type: bg1
  friendly_name: "Hall socket"
  timeout: 2

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant Bot commented Mar 29, 2020

Hey there @Danielhiversen, @felipediel, mind taking a look at this pull request as its been labeled with a integration (broadlink) you are listed as a codeowner for? Thanks!

@b4dpxl b4dpxl mentioned this pull request Mar 29, 2020
Closed
7 tasks
@MartinHjelmare MartinHjelmare changed the title Added support for Broadlink BG1 devices Add support for Broadlink BG1 devices Mar 29, 2020
@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Apr 29, 2020

Please rebase on latest dev branch and solve the merge conflict.

@felipediel do you have time to review?

@felipediel
Copy link
Copy Markdown
Contributor

felipediel commented Apr 29, 2020

@MartinHjelmare Could we hold this PR a little longer? As soon as I finish implementing config entries I will come back here to help review and integrate this code.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Apr 29, 2020

Ok. 👍

felipediel
felipediel reviewed Apr 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@felipediel felipediel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not yet my final review, just a warning about a problem I recently discovered in this integration.

Comment thread homeassistant/components/broadlink/switch.py
Comment on lines +460 to +462
try:
self._device.set_power(self._slot, packet)
except (socket.timeout, ValueError) as error:
Copy link
Copy Markdown
Contributor

@felipediel felipediel Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic. You are not handling authentication errors and a wide variety of socket errors. If authentication fails once, the device will stop working until you restart Home Assistant.

Copy link
Copy Markdown
Contributor Author

@b4dpxl b4dpxl Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was duplicated straight from the existing Switch and Slot implementations, so if this is an issue then those need fixing as well? In fact, looking (briefly) again I don't think this method is even used in this class :|

However, I am missing exception handling and retry in the _turn_on_off() method which does basically the same duty 🤦 - I'll fix that.

Re auth, the nearest thing I can see from the current implementation is that the auth method just catches the generic OSError. It would be easy enough to change socket.timeout to catch OSError instead, which should cover everything, and the polled update should handle re-authenticating. Would that work?

Copy link
Copy Markdown
Contributor

@felipediel felipediel Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this problem is real and it is everywhere. I am working on a solution that uses helper functions to handle exceptions. In the end it will be easier for you. I'll let you know when I finish.

Comment thread homeassistant/components/broadlink/switch.py
Comment on lines +522 to +527
try:
resp = self._device.get_state()
if resp is not None:
states = {"s1": resp["pwr1"], "s2": resp["pwr2"]} # Left # Right
_LOGGER.debug(states)
except (socket.timeout, ValueError) as error:
Copy link
Copy Markdown
Contributor

@felipediel felipediel Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Copy Markdown
Contributor Author

@b4dpxl b4dpxl Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above, this logic was duplicated from existing code.

@felipediel
Copy link
Copy Markdown
Contributor

felipediel commented May 11, 2020

Hey @b4dpxl, what's up!

This is the bug fix that I mentioned earlier. From now on we will use the BroadlinkDevice class as a bridge to communicate with the API. Use device.async_request() whenever you want to send a request to the device.

We still need to wait for this bug fix to be merged before I start working on the config entries. But if you adapt your code to use this pattern in the meantime it will be halfway there.

@b4dpxl
Copy link
Copy Markdown
Contributor Author

b4dpxl commented May 20, 2020

@felipediel I'll take a look. So far all I've managed to do is completely screw up my local version trying to rebase it :(

@b4dpxl b4dpxl mentioned this pull request Jun 4, 2020
Closed
20 tasks
@b4dpxl
Copy link
Copy Markdown
Contributor Author

b4dpxl commented Jun 4, 2020

Replaced with #36449

@b4dpxl b4dpxl closed this Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@felipediel felipediel felipediel left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Danielhiversen Danielhiversen

Labels

cla-signed integration: broadlink

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@b4dpxl @MartinHjelmare @felipediel @Danielhiversen @homeassistant